Fix bugs with BDD codegen#4666
Merged
Merged
Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
vcjana
approved these changes
May 14, 2026
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
ysaito1001
approved these changes
May 15, 2026
Collaborator
ysaito1001
left a comment
There was a problem hiding this comment.
Left comments for potential doc cleanups, LGTM otherwise.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Was testing our BDD implementation against the updated service models with BDD traits. Found two bugs that impacted
dynamodbands3control.Description
DynamoDB endpoint resolver
DynamoDB's BDD exercises two codegen patterns that no other service uses, and both
produced uncompilable Rust:
stringEquals(multipartTemplate, optionalRef)with a{Var#attr}interpolation on anassigned variable (condition 8). The template's dynamic-part path was hard-coded to the
tree-mode generator and lost BDD's Option-unwrap tracking; the comparison wrapper
inherited Borrowed ownership, producing
&mut Option<&String>instead of&mut Option<String>.parseArn(FirstArn)whereFirstArn = getAttr(ResourceArnList, "[0]")(conditions19/20). The list-index assignment eagerly cloned (
Option<String>), severing the lifetimechain back to the parameter;
parseArnthen producedArn<'closure>that couldn't fit thecontext's outer-lifetime slot
Fix (LiteralGenerator.kt, BddExpressionGenerator.kt, EndpointBddGenerator.kt):
LiteralGeneratornow accepts anexprGeneratorcallback so BDD-mode template dynamicparts retain Option-unwrap tracking; the inner String-typed parameter case renders as
name.as_deref().unwrap_or_default().visitStringEqualsswitches toOwnership.Ownedwhen wrapping a Literal in&mut Some(...), so multipart templates produce an owned String.isBorrowedStrflag onAnnotatedRefmarks SSA variables whose assignment is aborrow-preserving chain (Reference/GetAttr rooted at a parameter). Those context fields
are emitted as
Option<&'a str>, and the assignment uses.first().map(|s| s.as_str())rather than
.first().cloned() ... .into(). The parameter → SSA variable → parseArnresult lifetime chain is preserved end-to-end.
S3 Control parameter / SSA-variable name collision
S3 Control's BDD declares parameter
ResourceArnand SSA variableresourceArn, distinctidentifiers in Smithy that both rust-name to
resource_arn. Per-condition closures emitlet resource_arn = &mut context.resource_arn;, whichshadows the top-level
let resource_arn = ¶ms.resource_arn;. Any condition body that referenced the parameter (e.g.parseArn(ResourceArn)) saw the uninitialized context variable instead.Fix (EndpointBddGenerator.kt, BddExpressionGenerator.kt, Util.kt,
ExpressionGenerator.kt):
AnnotatedRefs.fromdetects when an SSA variable's rust-name collides with an existingentry and disambiguates by appending
_ctx_N. For S3 Control:resourceArnbecomes
resource_arn_ctx_1the parameter keeps the unmodifiedresource_arn.nameByOriginalrename map is threaded throughAnnotatedRefsandContext, and everycode-emission site (BDD condition closures and the tree-mode
ExpressionGeneratorusedfor endpoint result construction) resolves identifiers via
refs.resolveName(id)so thedisambiguated name propagates consistently.
Testing
Tested that the models built correctly and that their endpoint tests pass. Updated the
dynamodbands3-controlmodels in the repo to haveendpointBddtraits since those were the two that caused issues.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.